- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15k
[ValueTracking] Teach isGuaranteedNotToBeUndefOrPoison about splats #163570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ValueTracking] Teach isGuaranteedNotToBeUndefOrPoison about splats #163570
Conversation
Splats include two poison values, but only the poison-ness of the splatted value actually matters.
| @llvm/pr-subscribers-clang @llvm/pr-subscribers-llvm-analysis Author: Cullen Rhodes (c-rhodes) ChangesSplats include two poison values, but only the poison-ness of the splatted value actually matters. Full diff: https://github.com/llvm/llvm-project/pull/163570.diff 2 Files Affected: 
 diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 9655c886f4441..b0016c36bf5da 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -7678,6 +7678,7 @@ static bool isGuaranteedNotToBeUndefOrPoison(
         return true;
     }
 
+    Value *Splat;
     if (!::canCreateUndefOrPoison(Opr, Kind,
                                   /*ConsiderFlagsAndMetadata=*/true)) {
       if (const auto *PN = dyn_cast<PHINode>(V)) {
@@ -7695,6 +7696,10 @@ static bool isGuaranteedNotToBeUndefOrPoison(
         }
         if (IsWellDefined)
           return true;
+      } else if (isa<ShuffleVectorInst>(Opr) && (Splat = getSplatValue(Opr))) {
+        // For splats we only need to check the value being splatted.
+        if (OpCheck(Splat))
+          return true;
       } else if (all_of(Opr->operands(), OpCheck))
         return true;
     }
diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index 559a0b724f383..bb0280ee69cfd 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -1091,6 +1091,16 @@ TEST_F(ValueTrackingTest, isGuaranteedNotToBeUndefOrPoison) {
   }
 }
 
+TEST_F(ValueTrackingTest, isGuaranteedNotToBeUndefOrPoison_splat) {
+  parseAssembly(
+      "define <4 x i32> @test(i32 noundef %x) {\n"
+      "  %ins = insertelement <4 x i32> poison, i32 %x, i32 0\n"
+      "  %A = shufflevector <4 x i32> %ins, <4 x i32> poison, <4 x i32> zeroinitializer\n"
+      "  ret <4 x i32> %A\n"
+      "}");
+  EXPECT_TRUE(isGuaranteedNotToBeUndefOrPoison(A));
+}
+
 TEST_F(ValueTrackingTest, isGuaranteedNotToBeUndefOrPoison_assume) {
   parseAssembly("declare i1 @f_i1()\n"
                 "declare i32 @f_i32()\n"
 | 
| You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions c,cpp -- clang/test/CodeGen/arm-mve-intrinsics/dup.c clang/test/Headers/wasm.c llvm/lib/Analysis/ValueTracking.cpp llvm/unittests/Analysis/ValueTrackingTest.cpp --diff_from_common_commit
 View the diff from clang-format here.diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index bb0280ee6..2c22a7a99 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -1092,12 +1092,12 @@ TEST_F(ValueTrackingTest, isGuaranteedNotToBeUndefOrPoison) {
 }
 
 TEST_F(ValueTrackingTest, isGuaranteedNotToBeUndefOrPoison_splat) {
-  parseAssembly(
-      "define <4 x i32> @test(i32 noundef %x) {\n"
-      "  %ins = insertelement <4 x i32> poison, i32 %x, i32 0\n"
-      "  %A = shufflevector <4 x i32> %ins, <4 x i32> poison, <4 x i32> zeroinitializer\n"
-      "  ret <4 x i32> %A\n"
-      "}");
+  parseAssembly("define <4 x i32> @test(i32 noundef %x) {\n"
+                "  %ins = insertelement <4 x i32> poison, i32 %x, i32 0\n"
+                "  %A = shufflevector <4 x i32> %ins, <4 x i32> poison, <4 x "
+                "i32> zeroinitializer\n"
+                "  ret <4 x i32> %A\n"
+                "}");
   EXPECT_TRUE(isGuaranteedNotToBeUndefOrPoison(A));
 }
 
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Seems like a sensible change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Perhaps we can also teach this function with single-source shufflevectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are two clang tests that need to be updated as well.
| } | ||
| if (IsWellDefined) | ||
| return true; | ||
| } else if (auto *Splat = isa<ShuffleVectorInst>(Opr) ? getSplatValue(Opr) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious about how auto actually works here when selecting between two possibilities? It could choose nullptr, in which case it might be void * I presume? I can only assume it creates the type based on the first option, i.e. getSplatValue. I just wasn't sure whether the code compiles because of luck or an actual C++ specification based on ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The precise rules for determining the result type of a ternary operator are exceedingly complicated, but the tl;dr is that nullptr is of type nullptr_t, which implicitly converts to all pointer types, and that's what makes this work.
| LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/38197 Here is the relevant piece of the build log for the reference | 
…lvm#163570) Splats include two poison values, but only the poison-ness of the splatted value actually matters.
…lvm#163570) Splats include two poison values, but only the poison-ness of the splatted value actually matters.
Splats include two poison values, but only the poison-ness of the splatted value actually matters.